Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DietPi-Globals | G_CONFIG_INJECT: Auto escaping of special characters #2227

Merged
merged 18 commits into from
Nov 8, 2018

Conversation

MichaIng
Copy link
Owner

@MichaIng MichaIng commented Nov 7, 2018

Status: Testing

  • Check all DietPi code internal calls for extended regular expression use
  • Check all DietPi code internal calls for escaping within $2
  • Check newline support
  • Changelog

Reference: https://github.com/Fourdee/DietPi/issues/2215

Commit list/description:

  • DietPi-Globals | G_CONFIG_INJECT: Minor wording and error catching enhancements
    • Now shows sed error output in whiptail prompt as well
  • DietPi-Globals | G_CONFIG_INJECT: Do required special character escaping (on $2 only!) function internally
    • Only the usual single and double quote string escaping needs to be done, when not adding via variable.
  • DietPi-Globals | G_CONFIG_INJECT: Remove extended regular expression support
    • The ext regex support doesn't outweigh the way larger escaping effort and bug vulnerability. And even with basic regex only, (pattern1|pattern2) or [char]+ can be done via \(pattern1\|pattern2\) resp. [char]\+.
  • DietPi-Globals | G_CONFIG_INJECT: Use more individual option variable names

+ DietPi-Globals | G_CONFIG_INJECT: Minor wording and error catching enhancements
+ DietPi-Globals | G_CONFIG_INJECT: Do required special character escaping function internally
+ DietPi-Globals | G_CONFIG_INJECT: Remove extended regular expression support
@MichaIng MichaIng added this to the v6.18 milestone Nov 7, 2018
@MichaIng MichaIng self-assigned this Nov 7, 2018
@MichaIng MichaIng requested a review from Fourdee November 7, 2018 04:50
@MichaIng
Copy link
Owner Author

MichaIng commented Nov 7, 2018

@Fourdee
If you find time, would be great if you could play around with various special characters and try to cause a syntax error.

Should not be possible any more. If it survives enough tests, I will remove all the error handling from the function. Makes it ugly 😉.

+ DietPi-Globals | G_CONFIG_INJECT: Add escaping of forgotten "|", which is used as custom "sed" delimiter
@Fourdee
Copy link
Collaborator

Fourdee commented Nov 7, 2018

@MichaIng

Great work 👍

If you find time, would be great if you could play around with various special characters and try to cause a syntax error.

Hopefully i'll get some time tomorrow, will run some tests.

@MichaIng
Copy link
Owner Author

MichaIng commented Nov 7, 2018

@Fourdee
Sh*t, there is one big issue with automated escaping...

Since all special characters are taken literally, this breaks the use of regex expressions. So, e.g. G_CONFIG_INJECT 'setting1[[:blank:]]*=' ... is not possible any more... and there are many cases where settings allow optional-only spaces and other cases where we want to used regex.

Ah have an idea:

  • The regex pattern is usually only required for $1 and optional $4, where usually never a variable is inside. So there we can allow regex, skip escaping, thus require manual escaping if some special char is literally wanted.
  • The setting $2 usually contains a variable and we do not need regex there, they would not even be used of course by sed. So there we can do automated escaping.
  • It is not 100% consistent: Special chars need to be escaped in $1 and $4 then, but not $2. But for practical use this is the only solution I see that does not require more complicated handling, via e.g. "escaping function" to separately escape variables to pass to G_CONFIG_INJECT first or such...

+ DietPi-Config | G_CONFIG_INJECT: Auto-escape $2 settings argument only, to stay flexible with regex on matching pattern arguments
+ DietPi-Patch | Remove escaping due to G_CONFIG_INJECT changes
+ DietPi-PREP | Remove escaping due to G_CONFIG_INJECT changes
+ DietPi-LetsEncrypt | Replace ext regex with basic regex within G_CONFIG_INJECT arguments
+ DietPi-Patch | Remove forgotten escaping due to G_CONFIG_INJECT changes
+ DietPi-Software | Remove escaping and ext regex due to G_CONFIG_INJECT changes
@MichaIng
Copy link
Owner Author

MichaIng commented Nov 8, 2018

  • Without extended regular expressions, \| within grep is taken as or, instead of literally. This is the same issue as mentioned above with \(1\|2\) which will be interpreted as 1 or 2. So finally it is better to re-enable extended regular expressions, to allow these characters being interpreted literally at all. Otherwise with unlucky e.g. passwords, such escaped characters could be specially treated 🤣.
root@VM-Stretch:/tmp# grep "^[[:blank:]]*setting2 = \|hmmm\|" test
# Added by DietPi:
setting1 = "testing"
setting2 = |hmm|
root@VM-Stretch:/tmp# grep -E "^[[:blank:]]*setting2 = \|hmmm\|" test
root@VM-Stretch:/tmp# grep -E "^[[:blank:]]*setting2 = \|hmm\|" test
setting2 = |hmm|

€: 🈯️ Re-enabled ext regex. More escaping, but no other chance to avoid surprises

  • Newline is not supported within the setting. This is actually good, since we cannot predict when it is really wanted and when.
  • Solution is new option, to explicitly allow newlines. Simply replace \\n with \n then.

€: 🈯️ Enabled newline support via GCI_NEWLINE (G_CONFIG_INJECT)

  • Whiptail handles newline calls differently. It is not possible to escape them there.
  • whiptail --msgbox 'line1\\nline2' 10 230 echo, grep, sed, I guess POSIX conform is to read the backslash as escaped, thus literally, thus not expand the newline. But whiptail always does 🤔. However, this is minor...

€: Leave as is, very rare and visual only issue, no workaround possible, due to whiptail internal handling

+ DietPi-Globals | G_CONFIG_INJECT: Re-enable extended regular expression support, as otherwise \( \| \? \+ are specially treated and not literally...
+ DietPi-Globals | G_CONFIG_INJECT: Use more individual option variable names
+ Revert ext regex removal
+ Revert ext regex removal + within double quotes \\ still needs to be \\\\
+ syntax
+ syntax
+ syntax
+ DietPi-Software | Switch to new G_CONFIG_INJECT option var naming
+ Minor
+ CHANGELOG | G_CONFIG_INJECT internal special character escaping
@@ -7897,27 +7897,27 @@ _EOF_
# Set pretty URLs (without /index.php/) on Apache:
if (( ${aSOFTWARE_INSTALL_STATE[83]} >= 1 )); then

PRESERVE=1 G_CONFIG_INJECT "'htaccess.RewriteBase'" "'htaccess.RewriteBase' => '/owncloud'," $config_php "'overwrite.cli.url'"
CGI_PRESERVE=1 G_CONFIG_INJECT "'htaccess.RewriteBase'" "'htaccess.RewriteBase' => '/owncloud'," $config_php "'overwrite.cli.url'"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCI? 😃

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fourdee
👍 Whoopsie, fixed

typo
@Fourdee Fourdee merged commit 1e8b21b into dev Nov 8, 2018
@Fourdee
Copy link
Collaborator

Fourdee commented Nov 8, 2018

@MichaIng

Thanks Micha, great work 👍

Merged for testing.

@MichaIng MichaIng deleted the G_CONFIG_INJECT branch November 8, 2018 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants